-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
project: apply cargo fmt universally, verify in CI. #146
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be explicit, here's why I think this is a good idea:
- The default
cargo fmt
settings represent idiomatic style, that is used across most Rust projects - Automatic formatting tooling makes life easier for contributors
- Style enforcement in CI avoids (some) additional review roundtrips due to stylistic concerns
Thanks for adding motivating context. I should have done that in my PR description 👍 Your points echo the feelings I expressed in this comment. |
363133e
to
05f9557
Compare
Codecov Report
@@ Coverage Diff @@
## main #146 +/- ##
==========================================
- Coverage 71.60% 70.28% -1.32%
==========================================
Files 7 7
Lines 1729 1898 +169
==========================================
+ Hits 1238 1334 +96
- Misses 491 564 +73
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Btw, would it be possible to remove these coverage comments? they are put on every PR, even ones that don't touch anything related to tests. I think it's cool to have coverage reports you can browse online and look at graphs, but I don't think one needs to have a comment for each PR on how it changes the doc coverage. As for autoformatters, I think there are some options that make cargo fmt nicer, I will make some experiments and will then propose something. |
I'll look into that.
@est31 Does this PR need to wait for that adjustment? The benefit of machine formatting is that if you change the rules of the formatter, you can run it again. |
Let's try this first: #149 |
This commit checks in the result of running `cargo fmt` across the project tree.
This commit adds a job to the `ci.yml` workflow that ensures `cargo fmt` has been applied consistently.
05f9557
to
9d5cf2b
Compare
@est31 friendly ping on this? Would be nice for further changes to get the formatting done first. |
It's a pity that
|
I like the latter way, but I'm also glad that |
Thanks 👍 |
Self-explanatory. Applies
cargo fmt
universally, adds a CI job to ensure consistent formatting going forward. No functional changes.